Skip to content

Add apigateway and alb #52

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Apr 27, 2020
Merged

Add apigateway and alb #52

merged 5 commits into from
Apr 27, 2020

Conversation

fabianfett
Copy link
Member

@fabianfett fabianfett commented Apr 1, 2020

This builds on top of #46.

  • it reimplements: HTTPHeaders, HTTPMethod and HTTPResponseStatus in an AWS friendly way
  • it doesn't link NIO at all

@fabianfett fabianfett force-pushed the add-apigateway-and-alb branch 7 times, most recently from 3cf0a25 to 273ea5c Compare April 3, 2020 17:46
self.isBase64Encoded = try container.decode(Bool.self, forKey: .isBase64Encoded)

let body = try container.decode(String.self, forKey: .body)
self.body = body != "" ? body : nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

restore the PW from SNS?

)
let multiValueHeaders = singleValueHeaders.mapValues { [$0] }
self.headers = HTTPHeaders(multiValueHeaders)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

abstract with PW?

forKey: .queryStringParameters
)
self.queryStringParameters = singleValueQueryStringParameters.mapValues { [$0] }
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

abstract with PW?

debugDescription: #"Method "\#(rawMethod)" does not conform to allowed http method syntax defined in RFC 7230 Section 3.2.6"#
)
}
self.httpMethod = method
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

abstract with PW?

Copy link
Contributor

@tomerd tomerd Apr 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh actually isn't HTTPMethod Codable now? so this check could go there?

@tomerd
Copy link
Contributor

tomerd commented Apr 20, 2020

hey @fabianfett should we bring this over the finish line?

@fabianfett fabianfett force-pushed the add-apigateway-and-alb branch from 273ea5c to 25c8dd4 Compare April 22, 2020 15:49
@@ -54,7 +54,8 @@ public enum APIGateway {

public let queryStringParameters: [String: String]?
public let multiValueQueryStringParameters: [String: [String]]?
public let headers: HTTPHeaders
public let headers: [String: String]
public let multiValueHeaders: [String: [String]]
Copy link
Contributor

@tomerd tomerd Apr 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider a type alias for headers and multiValueHeaders, ie typealias HTTPHeaders = [String: String] and typealias HTTPMultiValueHeaders = [String: [String]] will make it easier to refactor in case we one day want to build functionality around them

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed...

XCTAssertEqual(event.isBase64Encoded, false)
// XCTAssertEqual(event.headers.count, 11)
XCTAssertEqual(event.headers?.count, 11)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove dead code

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which dead code do you mean?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry my bad

Copy link
Contributor

@tomerd tomerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this simplification allot. one suggestion re. type alias for headers for future use.

XCTAssertEqual(request.httpMethod, .POST)

// let todo = try request.decodeBody(Todo.self)
// XCTAssertEqual(todo.title, "a todo")
Copy link
Contributor

@tomerd tomerd Apr 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove dead code

// MARK: HTTPMethod

public typealias Headers = [String: String]
public typealias MultiValueHeaders = [String: [String]]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should those be named with prefix HTTP?


import class Foundation.JSONEncoder

// https://docs.aws.amazon.com/apigateway/latest/developerguide/set-up-lambda-proxy-integrations.html
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you know about API Gateway's v2 integration with lambda? https://aws.amazon.com/blogs/compute/building-better-apis-http-apis-now-generally-available/

By default, it's request/response shape is different, and requires a configuration to be compatible with the "1.0" shape you've got here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomerd let me see what I can do to get this to work today. Since v2 already is GA, there might be a chance for us never to support v1. 😉

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomerd Okay I added the API Gateway v2 syntax, which in my opinion is much nicer. I've already tried it with my own lambda quite extensively. Works great and the v2 syntax is much easier to use, if you ask me.

@fabianfett fabianfett force-pushed the add-apigateway-and-alb branch from 809008b to a00c605 Compare April 23, 2020 09:19
@fabianfett fabianfett force-pushed the add-apigateway-and-alb branch 2 times, most recently from 75ff881 to ae07717 Compare April 24, 2020 18:13
@fabianfett fabianfett force-pushed the add-apigateway-and-alb branch from ae07717 to cf2bc9a Compare April 24, 2020 18:18
Copy link
Member Author

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomerd This is ready for final review!

@fabianfett fabianfett requested a review from tomerd April 25, 2020 12:56
@tomerd
Copy link
Contributor

tomerd commented Apr 27, 2020

@fabianfett this looks like its ready to be merged, correct?

@tomerd tomerd merged commit 8509674 into master Apr 27, 2020
@tomerd tomerd deleted the add-apigateway-and-alb branch April 27, 2020 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants